feat: add the [approval_retention] config section - #183
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the approval_retention configuration feature, allowing users to specify which kinds of changes (such as whitespace, comments, formatting, string literals, or renames) can retain an existing approval instead of dismissing it. The changes include documentation in the README, config parsing logic, and comprehensive unit tests. The reviewer suggested a refactoring opportunity to simplify the repetitive ...Enabled helper methods in internal/config/config.go by consolidating them into a single parameterized helper function.
99a3371 to
b313dc9
Compare
|
Codeowners approval required for this PR: |
Confidence Score: 5/5The PR appears safe to merge because the new configuration is internally consistent, tested, and not yet consumed by production approval logic. The resolver methods match the documented defaults and overrides, malformed configuration falls back to the complete default configuration, and no production path currently changes behavior based on these settings.
|
| Filename | Overview |
|---|---|
| internal/config/config.go | Adds the approval-retention configuration model, nil-safe flag resolvers, and default section initialization with semantics consistent with the documented contract. |
| internal/config/config_test.go | Thoroughly covers absent, umbrella-disabled, umbrella-enabled, explicitly overridden, opt-in-only, and nil-receiver cases. |
| internal/app/approval_retention_test.go | Adds an end-to-end regression test confirming that an absent section and an explicitly disabled section preserve existing approval-dismissal behavior. |
| README.md | Documents all new options and accurately describes umbrella-following versus explicit-opt-in behavior. |
Reviews (1): Last reviewed commit: "feat: add the [approval_retention] confi..." | Re-trigger Greptile
Adds the config surface the retention rules will hang off. No behaviour change: nothing reads these flags yet. The individual flags are *bool rather than bool so that unset can be told apart from an explicit false. That is what lets the umbrella work in both directions: on with nothing set turns every following flag on, and on with one flag set to false turns everything except that one on. string_literals, renames and fetch_orphaned_approval are opt-in and never follow the umbrella. The first two can alter behaviour without changing the shape of the code an approver reviewed; the third reaches the network. An end-to-end test pins the inertness claim: a config with no section and one spelling the section out with everything off produce the same bytes.
b313dc9 to
86cc504
Compare
|
Closing this one. Deciding what counts as a change not worth re-reviewing turns out to be a judgement about a particular codebase rather than something this action should carry a default for. The normalizers here encode one set of conventions, and every consumer would inherit them along with the language-awareness they need to stay correct. Replacing it with a seam instead: the action hands out the hunks it is about to attribute to an approval, and a program named by the workflow answers which of them a reviewer has effectively already seen. Same outcome where somebody wants it, no opinion shipped by default. That is open as its own PR. No behaviour in this repo changes as a result of this close. |
1 participant
Related PR(s)
Base of the approval-retention series. These build on it:
fetch_orphaned_approvalSummary / Background
Adds an
[approval_retention]section tocodeowners.toml. No behaviour change — nothing reads these flags yet. This is the config surface the retention rules land behind.enabledis a kill switch, not a defaultEvery flag is off unless it is named.
enabledturns nothing on by itself; it switches the whole section off in one line, so a repository can suspend retention without editing seven values.enabledfalsetruetruetrueOne accessor reads that, so no call site can disagree with another:
Why the flags are
*boolBehaviour no longer needs it, now that unset and
falsemean the same thing. It is kept so the two can still be told apart: writingwhitespace = falseon purpose reads as a decision, and an absent value reads as one nobody has made yet.Opt-in flags
All of them, without exception.
string_literalsandrenamescan alter behaviour without changing the shape of the code an approver reviewed, andfetch_orphaned_approvalreaches the network, so those three are the ones worth thinking hardest about before switching on.Defaults
Everything is off, so nothing changes for anyone until they opt in. An end-to-end test pins that: a config with no section and one spelling the section out with everything off produce the same bytes.